-
Notifications
You must be signed in to change notification settings - Fork 0
Story/cite-177 There needs to be an import from Crossref #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…rossrefReferenceImportProcessor accordingly
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unaddressed comments
Looks fine. Once the Citesphere PR is ready to be tested, I'll deploy this. |
private ArticleMeta parseArticleMeta(Item item) { | ||
ArticleMeta meta = new ArticleMeta(); | ||
meta.setArticleTitle(item.getTitle().get(0)); | ||
meta.setShortTitle(String.join(", ", item.getTitle().subList(1, item.getTitle().size()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem right. Why would the short title be a concatenation of all other titles? If zotero doesn't allow additional titles, then we might just want to drop them for now.
contributors.addAll(mapPersonToContributor(item.getTranslator(), ContributionType.TRANSLATOR)); | ||
} | ||
// List of chair | ||
if(item.getChair() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that all contributor types that crossref knows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. crossref has 4 types of contributors - authors, editors, translators & chair
} | ||
meta.setContributors(contributors); | ||
|
||
meta.setAuthorNotesCorrespondence(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that the default?
meta.setLanguage(item.getLanguage()); | ||
ReviewInfo review = new ReviewInfo(); | ||
if (item.getReview() != null) { | ||
review.setFullDescription(item.getReview().getCompetingInterestStatement()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is full description the same as competing interest statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
private Reference mapSingleReference(edu.asu.diging.crossref.model.Reference itemRef) { | ||
Reference ref = new Reference(); | ||
ref.setAuthorString(itemRef.getAuthor()); | ||
ref.setContributors(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before, isn't that the default value?
...n/java/edu/asu/diging/citesphere/importer/core/service/parse/iterators/CrossRefIterator.java
Show resolved
Hide resolved
itemTypeMapping.put(Publication.NEWS_ITEM, ItemType.NEWSPAPER_ARTICLE); | ||
itemTypeMapping.put(Publication.PROCEEDINGS_PAPER, ItemType.CONFERENCE_PAPER); | ||
itemTypeMapping.put(Publication.DOCUMENT, ItemType.DOCUMENT); | ||
itemTypeMapping.put(Publication.BOOK, ItemType.BOOK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate of line 63
itemTypeMapping.put(Publication.EDITED_BOOK, ItemType.BOOK); | ||
itemTypeMapping.put(Publication.PROCEEDINGS_PAPER, ItemType.CONFERENCE_PAPER); | ||
itemTypeMapping.put(Publication.DISSERTATION, ItemType.THESIS); | ||
itemTypeMapping.put(Publication.BOOK_CHAPTER, ItemType.BOOK_SECTION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate
.../main/java/edu/asu/diging/citesphere/importer/core/service/impl/AbstractImportProcessor.java
Show resolved
Hide resolved
@@ -0,0 +1,157 @@ | |||
package edu.asu.diging.citesphere.importer.core.service.parse.crossref; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be moved to an impl
package
import edu.asu.diging.crossref.model.Person; | ||
|
||
@Component | ||
public class CrossRefParser implements ICrossRefParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs javadoc and tests
} | ||
|
||
@Test | ||
public void testParseJournalMeta_WithValidData() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test method names should follow our naming convention (documented in confluence)
...n/java/edu/asu/diging/citesphere/importer/core/service/parse/iterators/CrossRefIterator.java
Show resolved
Hide resolved
@Override | ||
public ContainerMeta parseJournalMeta(Item item) { | ||
ContainerMeta meta = new ContainerMeta(); | ||
meta.setContainerTitle(item.getContainerTitle().get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there more than one title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, crossref API is returning list of strings for title
@Override | ||
public ArticleMeta parseArticleMeta(Item item) { | ||
ArticleMeta meta = new ArticleMeta(); | ||
meta.setArticleTitle(item.getTitle().get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, are there more than one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, crossref API is returning list of strings for title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be a comment here
if(dateParts != null) { | ||
publicationDate.setPublicationDate(dateParts.get(2).toString()); | ||
publicationDate.setPublicationMonth(dateParts.get(1).toString()); | ||
publicationDate.setPublicationYear(dateParts.get(0).toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are all of these always set or could date be null for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
publicationDate will be null, only if the published date is missing. That was case for some citations. Hance, the check for dateParts is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, can dateParts.get(2)
return null
? If so, toString()
would throw an exception I believe?
Make it so, Jenkins. |
2 similar comments
Make it so, Jenkins. |
Make it so, Jenkins. |
I get compilation errors on Jenkins. I think there are some dependencies missing that are needed for the upgrade. Also, can you please update the Java version in the pom to 11? |
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]
into[x]
).Please provide a brief description of your ticket
The user should be able to search in crossref to references to import. whatever search results from Crossref a user selects should then be imported into citesphere. This could be all search results or just a few or one.
For now, let's put add an entry to the import menu and let the user select which group to import the results into from a dropdown menu.
The importing should be done from Citesphere Importer (not Citesphere), similar how entries from files are imported.
For querying crossref, this library should be used: https://github.com/diging/crossref-connect. However, this is not yet released into maven central, so talk to Julia before picking up this ticket.
... Put ticket description here and add link to ticket ...
https://diging.atlassian.net/browse/CITE-177
Are there any other pull requests that this one depends on?
diging/citesphere-messages#12
diging/citesphere#271
Anything else the reviewer needs to know?
... describe here ...